Skip to content

Approve functions used as default arguments #4699

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

cclauss
Copy link
Member

@cclauss cclauss commented Aug 30, 2021

Describe your change:

Use linter directive # noqa: B0008 for these functions because they set a seed value and BaseBackend is reused across all calls and LinearCongruentialGenerator.__init__() is only called once for each instance.

flake8-bugbear B008 Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.

  • Add an algorithm?
  • Fix a bug or typo in an existing algorithm?
  • Documentation change?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doctests that pass the automated testing.
  • All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

@cclauss cclauss requested a review from dhruvmanila August 30, 2021 19:25
@ghost ghost added awaiting reviews This PR is ready to be reviewed enhancement This PR modified some existing files labels Aug 30, 2021
@cclauss cclauss mentioned this pull request Aug 30, 2021
14 tasks
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is intended, assign the function call to a module-level variable and use that variable as a default value.

Why don't we do what the linter is suggesting us to? I hate adding ignore directives throughout the code. If not, then we should atleast explain in a comment as to why that directive was added.

@ghost ghost added awaiting changes A maintainer has requested changes to this PR and removed awaiting reviews This PR is ready to be reviewed labels Aug 31, 2021
The default value for **seed** is the result of a function call which is not normally recommended and causes flake8-bugbear to raise a B008 error. However, in this case, it is accptable because `LinearCongruentialGenerator.__init__()` will only be called once per instance and it ensures that each instance will generate a unique sequence of numbers.
@ghost ghost added awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass and removed awaiting changes A maintainer has requested changes to this PR labels Aug 31, 2021
The default value for **backend** is the result of a function call which is not normally recommended and causes flake8-bugbear to raise a B008 error. However, in this case, it is accptable because `Aer.get_backend()` is called when the function is definition and that same backend is then reused for function calls.
Copy link
Member

@dhruvmanila dhruvmanila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the explanation should be a comment instead of adding in a docstring as it is intended only for developers instead of end-users, otherwise LGTM.

@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label Aug 31, 2021
@cclauss
Copy link
Member Author

cclauss commented Aug 31, 2021

Comment above function definition or below? If below, before docstring or after?

@dhruvmanila
Copy link
Member

Above the directive which means above the function definition.

@ghost ghost added the awaiting reviews This PR is ready to be reviewed label Aug 31, 2021
@ghost ghost removed the tests are failing Do not merge until tests pass label Aug 31, 2021
@cclauss cclauss merged commit ef98271 into TheAlgorithms:master Aug 31, 2021
@ghost ghost removed the awaiting reviews This PR is ready to be reviewed label Aug 31, 2021
@cclauss cclauss deleted the function-calls-as-default-arguments branch August 31, 2021 05:56
shermanhui pushed a commit to shermanhui/Python that referenced this pull request Oct 22, 2021
* Approve functions used as default argumenets

* The default value for **seed** is the result of a function call

The default value for **seed** is the result of a function call which is not normally recommended and causes flake8-bugbear to raise a B008 error. However, in this case, it is accptable because `LinearCongruentialGenerator.__init__()` will only be called once per instance and it ensures that each instance will generate a unique sequence of numbers.

* The default value for **backend** is the result of a function call

The default value for **backend** is the result of a function call which is not normally recommended and causes flake8-bugbear to raise a B008 error. However, in this case, it is accptable because `Aer.get_backend()` is called when the function is definition and that same backend is then reused for function calls.

* Update linear_congruential_generator.py

* Update ripple_adder_classic.py

* Update ripple_adder_classic.py

* Update ripple_adder_classic.py

* Update ripple_adder_classic.py

* Update ripple_adder_classic.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This PR modified some existing files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants